Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ci: use clang 18 from Homebrew on aarch64 #6153

Merged
merged 1 commit into from
Apr 25, 2024
Merged

ci: use clang 18 from Homebrew on aarch64 #6153

merged 1 commit into from
Apr 25, 2024

Conversation

jakubgs
Copy link
Member

@jakubgs jakubgs commented Mar 28, 2024 •
edited
Loading

Not sure if LDFLAGS and CPPFLAGS are absolutely necessary but Brew docs recommend it.

Depends on:

@jakubgs jakubgs added the CI label Mar 28, 2024
@jakubgs jakubgs self-assigned this Mar 28, 2024
@jakubgs jakubgs force-pushed the ci/clang-17 branch 2 times, most recently from f1dc826 to f98e923 Compare March 28, 2024 16:22
@jakubgs jakubgs requested a review from tersec March 28, 2024 16:37
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 28, 2024 •
edited
Loading

Unit Test Results

         9 files  ±0    1 115 suites  ±0   34m 3s ⏱️ +15s
  4 245 tests ±0    3 898 ✔️ ±0  347 💤 ±0  0 ❌ ±0 
16 929 runs  ±0  16 531 ✔️ ±0  398 💤 ±0  0 ❌ ±0 

Results for commit 67060ca. ± Comparison against base commit c7d5ad7.

♻️ This comment has been updated with latest results.

@jakubgs jakubgs marked this pull request as ready for review March 29, 2024 09:08
@jakubgs
Copy link
Member Author

jakubgs commented Apr 3, 2024

Doesn't seem to like llvm@17:

Building: build/test_libnimbus_lc
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:54:5: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
   54 |     buffer[size] = '\0';
      |     ^~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:96:24: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
   96 |         printf("%02x", bytes_[i]);
      |                        ^~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:105:24: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  105 |         printf("%02x", bytes_[i]);
      |                        ^~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:119:17: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  119 |             if (value.bytes[i]) {
      |                 ^~~~~~~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:130:70: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  130 |             uint16_t temp = (uint16_t) ((uint16_t) remainder << 8) | value.bytes[i];
      |                                                                      ^~~~~~~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:131:13: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  131 |             value.bytes[i] = (uint8_t) (temp / 10);
      |             ^~~~~~~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:134:9: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  134 |         weiString[o++] = '0' + (char) remainder;
      |         ^~~~~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:137:9: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  137 |         weiString[o++] = '0';
      |         ^~~~~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:144:26: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  144 |             printf("%c", weiString[--o]);
      |                          ^~~~~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:148:21: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  148 |     while (z < o && weiString[z] == '0') {
      |                     ^~~~~~~~~
beacon_chain/libnimbus_lc/test_libnimbus_lc.c:154:26: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
  154 |             printf("%c", weiString[--o]);
      |                          ^~~~~~~~~
11 errors generated.

@tersec
Copy link
Contributor

tersec commented Apr 3, 2024

Well, -Wno-unsafe-buffer-usage would solve this.

https://stackoverflow.com/questions/77017567/how-to-fix-code-to-avoid-warning-wunsafe-buffer-usage

https://www.devgem.io/posts/how-to-address-the-wunsafe-buffer-usage-warning-in-clang

It seems like it's a rather zealous warning, overzealous maybe even. The -Werror is from nimbus-eth2's own Makefile.

But short term, probably disable that warning, which comes from -Weverything, and get this PR mergeable.

@etan-status
Copy link
Contributor

May also need -Wno-unknown-warning-option so that the older clang which doesn't know yet about this new warning won't bail.

@jakubgs jakubgs force-pushed the ci/clang-17 branch 2 times, most recently from ec60f01 to 67060ca Compare April 16, 2024 13:22
@jakubgs
Copy link
Member Author

jakubgs commented Apr 16, 2024 •
edited
Loading

Okay, now only x86_64 has some bizarre issue:

/var/folders/ks/5chrbv156ybfyr8g02znf0lr0006vn/T/assembly-e81b13.s:1472:1: error: invalid CFI advance_loc expression
.cfi_adjust_cfa_offset 8
^
/var/folders/ks/5chrbv156ybfyr8g02znf0lr0006vn/T/assembly-e81b13.s:6555:1: error: invalid CFI advance_loc expression
.cfi_adjust_cfa_offset 8
^
/var/folders/ks/5chrbv156ybfyr8g02znf0lr0006vn/T/assembly-e81b13.s:6673:1: error: invalid CFI advance_loc expression
.cfi_adjust_cfa_offset 8
...

I'll try to see if it's consistent across different hosts.

@jakubgs
Copy link
Member Author

jakubgs commented Apr 17, 2024

It appears this issue is not specific to maci7-02, and it fails the same way on maci7-01:

07:43:46  Building: build/ncli_db
07:43:49  /var/folders/kb/g1563gf912j8s81z0klrpxrr0006vn/T/assembly-f4eb15.s:1472:1: error: invalid CFI advance_loc expression
07:43:49  .cfi_adjust_cfa_offset 8
07:43:49  ^
07:43:49  /var/folders/kb/g1563gf912j8s81z0klrpxrr0006vn/T/assembly-f4eb15.s:6555:1: error: invalid CFI advance_loc expression
07:43:49  .cfi_adjust_cfa_offset 8
07:43:49  ^

https://ci.status.im/job/nimbus-eth2/job/platforms/job/macos/job/x86_64/job/PR-6153/10/console

@tersec
Copy link
Contributor

tersec commented Apr 17, 2024

@tersec
Copy link
Contributor

tersec commented Apr 17, 2024

Also https://reviews.llvm.org/D153167 is the clang/LLVM change triggering this. WINE was hit too: https://bugs.winehq.org/show_bug.cgi?id=55863

@tersec
Copy link
Contributor

tersec commented Apr 17, 2024

The underlying issue doesn't seem to show up on anything but macOS aarch64, so one option might be to only use Homebrew clang 17 on that aarch64, not x86, macOS.

@jakubgs
Copy link
Member Author

jakubgs commented Apr 17, 2024

Of course, just my luck to get edge cases like this.

I wonder if it's all versions of LLVM 17 or if there's some specific minor release that isn't affected. If so we could try to target a specific one, we could do that with Nix instead of Brew, which would also be more effectively pinned.

@tersec
Copy link
Contributor

tersec commented Apr 17, 2024

Of course, just my luck to get edge cases like this.

I wonder if it's all versions of LLVM 17 or if there's some specific minor release that isn't affected. If so we could try to target a specific one, we could do that with Nix instead of Brew, which would also be more effectively pinned.

llvm/llvm-project@0b06727 which is the first commit here shows up in https://github.com/llvm/llvm-project/releases/tag/llvmorg-17.0.0-rc1 so probably it affects every version of LLVM 17.

@jakubgs
Copy link
Member Author

jakubgs commented Apr 17, 2024

one option might be to only use Homebrew clang 17 on that aarch64, not x86, macOS.

I'm cringing just thinking about it. What about going further to 18?

@tersec
Copy link
Contributor

tersec commented Apr 17, 2024

one option might be to only use Homebrew clang 17 on that aarch64, not x86, macOS.

I'm cringing just thinking about it. What about going further to 18?

Sure, why not

@jakubgs
Copy link
Member Author

jakubgs commented Apr 20, 2024

It appears LLVM 18 is not yet available in Homebrew:

@tersec
Copy link
Contributor

tersec commented Apr 20, 2024

I suspect clang 18 doesn't/won't compile BLST on macOS x86 for the same reason clang 17 doesn't. It doesn't seem like this is a clang 17 bug, but a new constraint they're checking for that target platform. There's still no externally visible evidence BLST is responding to/caring about this particular breakage, on their end, but a likely eventual resolution might be that BLST fixes this, nimbus-eth2 updates to that BLST release, and then clang 17 works for nimbus-eth2.

@jakubgs
Copy link
Member Author

jakubgs commented Apr 23, 2024 •
edited
Loading

You're right, it still fails even with LLVM 18 on x86_64:


[2024-04-23T08:47:20.443Z] Building: build/ncli_db
[2024-04-23T08:47:26.785Z] /var/folders/ks/5chrbv156ybfyr8g02znf0lr0006vn/T/assembly-5eeab4.s:1472:1: error: invalid CFI advance_loc expression
[2024-04-23T08:47:26.785Z] .cfi_adjust_cfa_offset 8
[2024-04-23T08:47:26.785Z] ^
[2024-04-23T08:47:26.785Z] /var/folders/ks/5chrbv156ybfyr8g02znf0lr0006vn/T/assembly-5eeab4.s:6555:1: error: invalid CFI advance_loc expression
[2024-04-23T08:47:26.785Z] .cfi_adjust_cfa_offset 8
[2024-04-23T08:47:26.785Z] ^

https://ci.status.im/job/nimbus-eth2/job/platforms/job/macos/job/x86_64/job/main/job/PR-6153/11/console

@jakubgs jakubgs changed the title ci: use clang 17 installed from Homebrew ci: use clang 18 from Homebrew on aarch64 Apr 24, 2024
@jakubgs
ci: use clang 18 from Homebrew on aarch64
824bb17 
Not sure if `LDFLAGS` and `CPPFLAGS` are absolutely necessary but Brew docs recommend it.

Depends on:
status-im/infra-ci@67fafcb5
status-im/status-jenkins-lib#90

Signed-off-by: Jakub Sokołowski <jakub@status.im>
@jakubgs jakubgs merged commit 824bb17 into unstable Apr 25, 2024
12 checks passed
@jakubgs jakubgs deleted the ci/clang-17 branch April 25, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tersec tersec tersec approved these changes

Assignees

@jakubgs jakubgs

Labels
CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakubgs @tersec @etan-status